Implement a ComponentGraphGenerator from the assets API
#16
Conversation
github-actions
bot
added
part:tests
There was a problem hiding this comment.
Pull Request Overview
This PR implements a ComponentGraphGenerator class that generates component graphs for microgrids using the Assets API. The implementation replaces template/boilerplate code with production functionality.
- Adds
ComponentGraphGeneratorclass for fetching and constructing microgrid component graphs from the Assets API - Removes template code (
delete_mefunction) and adds real test coverage for formula generation - Adds git-based dependencies for
frequenz-microgrid-component-graphandfrequenz-client-assets
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
src/frequenz/gridpool/_graph_generator.py |
New module implementing ComponentGraphGenerator class with graph generation logic |
src/frequenz/gridpool/__init__.py |
Exports ComponentGraphGenerator and fixes docstring typo |
tests/test_gridpool.py |
Replaces template tests with actual test for formula generation using mocked Assets API |
pyproject.toml |
Adds git-based dependencies and removes TODO comments |
Comments suppressed due to low confidence (1)
tests/test_gridpool.py:6
- Import of 'asyncio' is not used.
import asyncio
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
shsms
force-pushed
the
implement
branch
2 times, most recently
from
d257e1c to
9e9044c
Compare
shsms
force-pushed
the
implement
branch
2 times, most recently
from
cfaf500 to
5acf430
Compare
|
|
||
| graph = ComponentGraph[ | ||
| ElectricalComponent, ComponentConnection, ElectricalComponentId | ||
| ](components, cast(list[ComponentConnection], connections)) |
There was a problem hiding this comment.
Isn't this the return type of the connection method already? Why do we need this cast?
There was a problem hiding this comment.
No, the type is list[ComponentConnection | None] for connections. For components, it raises, for connections, it returns None. I made an issue for them: frequenz-floss/frequenz-client-assets-python#57
| """Initialize this instance. | ||
| Args: | ||
| server_url: The location of the microgrid API server in the form of a URL. |
| sign_secret: The secret to use for signing requests. | ||
| channel_defaults: The default options used to create the channel when not | ||
| specified in the URL. | ||
| connect: Whether to connect to the server as soon as a client instance is |
There was a problem hiding this comment.
What is the motivation for this? And how is it intended to be used?
There was a problem hiding this comment.
It is very useful for testing, when making mocks etc. But normally we'd want to connect immediately I guess.
It is a base client feature. Maybe this class should directly accept an assets service client, rather than the URL, etc.
There was a problem hiding this comment.
Maybe this class should directly accept an assets service client, rather than the URL
I think so, otherwise you need to expose all kind of settings here if you want to give the users more control over the client.
shsms
force-pushed
the
implement
branch
2 times, most recently
from
8f275e1 to
579bff7
Compare
Signed-off-by: Sahas Subramanian <[email protected]>
Signed-off-by: Sahas Subramanian <[email protected]>
Signed-off-by: Sahas Subramanian <[email protected]>
Signed-off-by: Sahas Subramanian <[email protected]>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
2 participants
No description provided.